Changing repo label depending on the host #168
Conversation
ycmjason
force-pushed
the
fix-github-link-label
branch
from
24ea10f to
fcb0042
Compare
ycmjason
force-pushed
the
fix-github-link-label
branch
from
fcb0042 to
c5a8f58
Compare
ycmjason
force-pushed
the
fix-github-link-label
branch
from
c5a8f58 to
34ccc58
Compare
|
Maybe this should be configurable? |
|
maybe I will keep the logic now since it is quite nice to have less config. But with the config would be a fallback? would that make sense? |
|
I'd just keep it simple and let the user configure it with a |
|
I personally prefer just setting the |
|
@inosik added the |
ycmjason
force-pushed
the
fix-github-link-label
branch
from
fa8eee9 to
bbec29f
Compare
ycmjason
force-pushed
the
fix-github-link-label
branch
from
bbec29f to
bb33ae3
Compare
lib/default-theme/NavLinks.vue
Outdated
|
|
||
| const repoHost = this.repoLink.match(/^https?:\/\/[^/]+/)[0] || 'github' | ||
| return ['GitHub', 'GitLab', 'Bitbucket'].find(platform => { | ||
| return repoHost.toLowerCase().includes(platform.toLowerCase()) |
There was a problem hiding this comment.
In client side you shouldn't use includes and find since Buble doesn't ployfill for that.
BTW, why define a Uppercase constant and then toLowerCase?
There was a problem hiding this comment.
Oh okay, I would change this to match instead.
Defining uppercase constants since they are what we want to show on the header, lowercase since we want to compare. But actually using regex with case insensitive flag and match would be nicer.
lib/default-theme/NavLinks.vue
Outdated
| repoLabel () { | ||
| if (this.$site.themeConfig.repoLabel) return this.$site.themeConfig.repoLabel | ||
|
|
||
| const repoHost = this.repoLink.match(/^https?:\/\/[^/]+/)[0] || 'github' |
There was a problem hiding this comment.
Since repo link start with https://, so the check match(/^https?:\/\/[^/]+/)[0] is duplicated.
in other words, the statements match(/^https?:\/\/[^/]+/)[0] is dangerous since match would return undefined!
There was a problem hiding this comment.
This is true. Will fix this later.
.gitignore
Outdated
| @@ -4,3 +4,4 @@ node_modules | |||
| .temp | |||
| vuepress | |||
| TODOs.md | |||
| *.sw* | |||
There was a problem hiding this comment.
For vim swap files.
There was a problem hiding this comment.
You should add it at your project instead of here.
There was a problem hiding this comment.
Just thought it would be great since there might be other contributors who use vim.
There was a problem hiding this comment.
Hmmm, so should I put all the different needs here?
There was a problem hiding this comment.
It's up to you guys. I could remove it if you don't like it.
I just feel that it's not something that will hurt to add and might make other vim contributors lives easier. If there are other contributors who uses other editors which has some other ignore files, I couldn't see why we shouldn't facilitate it.
There was a problem hiding this comment.
It doesn't depend on me. please do not kidnap thinking.
What I want to say it that you can set this to a global git configuration instead of duplicating it everywhere.
There was a problem hiding this comment.
That is certainly not true for me. I am happy to remove that if this is not wanted.
|
@ulivz I have made some changes. Please review them again. Here are some changes:
|
|
Hmmmm.... this commit introduced a very obvious bug:
Fixed at f55fa00. |
Fixing #167